Fix Domain Admin prerequisite under PowerShell 7 AD compatibility shim (v1.3.3) - #48
Merged
Merged
Conversation
…ase v1.3.3 Under PowerShell 7, an RSAT ActiveDirectory module that is not Core-native loads through the Windows PowerShell compatibility shim and returns deserialized objects, so SIDs come back as strings. Test-TierModelPrerequisites compared a deserialized SID against a live SecurityIdentifier, so the Domain Admin membership check never matched and reported "Domain Admin membership required for deployment operations" even for genuine Domain Admins. - Import the ActiveDirectory module with -SkipEditionCheck so PowerShell 7 loads it in-process (native objects, no deserialization) in both the module-availability check and the Domain Admin check. - Add a fail-fast guard that detects the compatibility-shim condition (the domain SID returned as a string) and stops with clear remediation, preventing a silent deployment that would resolve empty SIDs into URA/GPO policy. - Bump module version 1.3.2 -> 1.3.3 and the version-assertion tests. - Add a compat-shim unit test to Unit.Prerequisites.Tests.ps1 (automated suite 1,652 -> 1,653, 0 failures). Refresh README counts and docs/test-coverage.md (Test-TierModelPrerequisites 85.12% -> 85.40%). CHANGELOG entry for 1.3.3. Lab-validated end to end: full -FullDeployment with all -Include options and -EnableAuditing (707 actions, 0 errors, all SIDs resolving) plus a full audit (418 checks, COMPLIANT, 0 drift). Closes #47
The cSpell dictionary was consolidated into .vscode/settings.json (personal, gitignored) per the note in that file. Removing the tracked root cspell.json makes the Code Spell Checker configuration personal-only rather than a shared repository dictionary.
Copilot started reviewing on behalf of
Joel Platek (VAsHachiRoku)
August 31, 2026 08:33
View session
There was a problem hiding this comment.
Pull request overview
This PR addresses a PowerShell 7 + RSAT ActiveDirectory compatibility issue where the Domain Admin prerequisite can false-fail (and SID resolution can silently break) when AD cmdlets are executed via the Windows PowerShell compatibility shim that returns deserialized objects.
Changes:
- Import
ActiveDirectorywith-SkipEditionCheckto avoid deserialized AD objects under PowerShell 7, and add a shim-detection guard. - Bump module version to
1.3.3and update version assertion tests accordingly. - Add a unit test for the compat-shim scenario and refresh docs/changelog/test-counts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/TierModel/public/Test-TierModelPrerequisites.ps1 | Imports AD module with -SkipEditionCheck and adds shim detection to prevent broken SID behavior. |
| tests/Unit.Prerequisites.Tests.ps1 | Adds unit coverage for the deserialized-object (compat shim) failure mode. |
| modules/TierModel/TierModel.psd1 | Bumps module version to 1.3.3. |
| tests/Unit.ModuleManifest.Tests.ps1 | Updates expected manifest version to 1.3.3. |
| tests/Integration.Module.Tests.ps1 | Updates expected loaded module version to 1.3.3. |
| CHANGELOG.md | Documents the fix in the 1.3.3 release notes. |
| docs/test-coverage.md | Updates coverage notes and adds a v1.3.3 measurement summary. |
| README.md | Updates reported test counts/date. |
| cspell.json | Removes tracked cspell config as housekeeping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+328
to
+333
| if ($adShimDetected) { | ||
| $result.Valid = $false | ||
| $null = $result.Errors.Add("ActiveDirectory module is loaded through the Windows PowerShell compatibility shim (deserialized objects); SID resolution would break URA and GPO deployment.") | ||
| $null = $result.Remediation.Add("Run the deployment from a host with a PowerShell 7-native RSAT ActiveDirectory module (Windows 11 / Windows Server 2022 or later), or run under Windows PowerShell 5.1.") | ||
| } | ||
| elseif (Get-Module ActiveDirectory) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #47 — the Domain Admin membership prerequisite reported "Domain Admin membership required for deployment operations" even for accounts that are members of Domain Admins.
Root cause
Under PowerShell 7, an RSAT ActiveDirectory module that is not Core-native (for example on a Windows Server 2016 host) is loaded through the Windows PowerShell compatibility shim (
WinPSCompatSession). The shim returns deserialized objects, so SIDs come back as plain strings instead ofSystem.Security.Principal.SecurityIdentifier.Test-TierModelPrerequisitescompared a deserialized.SIDagainst a liveSecurityIdentifier:That comparison never matches when the object is deserialized, so the check concluded the user was not a Domain Admin.
The same deserialization also affects the SID resolution used to build GPO URA and restricted-groups policy (
.SID.Value/.objectSid.Valuereturn empty under the shim), so fixing only the membership check would have allowed a deployment to proceed and write empty principals into policy. This change prevents that.Changes
-SkipEditionCheckso PowerShell 7 loads it in-process (native objects, no deserialization) — in both the module-availability check and the Domain Admin check. This matches the existing GroupPolicy import.Unit.Prerequisites.Tests.ps1.docs/test-coverage.md, andCHANGELOG.md.Scope
Production changes are limited to
modules/TierModel/public/Test-TierModelPrerequisites.ps1, plus the version bump and test/doc updates. No deployment or audit logic changed.Validation
-FullDeployment -IncludeMsa -IncludeGmsa -IncludeDmsa -IncludeWinLaps -EnableAuditing→ 707 actions applied, 0 errors, all SID-based ACLs resolving.-UseWindowsPowerShell): confirmed the guard fires, and native load (-SkipEditionCheck) resolves SIDs correctly.Coverage
Test-TierModelPrerequisites.ps185.12% → 85.40% (the new guard is fully covered). Overall aggregate unchanged (~88.93%); all module-scope files remain above the 80% CI gate.Additional housekeeping (separate commit)
Removes the tracked root
cspell.json; the Code Spell Checker dictionary now lives in the personal (gitignored).vscode/settings.json. Drop that commit if the release should be limited strictly to the #47 fix.Closes #47